Un-nest Ed25519 cryptocb cases from HAVE_CURVE25519 - #493
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a conditional-compilation bug in the client crypto callback dispatch so Ed25519 operations are available even when Curve25519 is disabled (they are independent wolfSSL build options). It also adds a targeted regression test and CI coverage to prevent future guard/dispatch regressions in Ed25519 offload.
Changes:
- Un-nests the Ed25519
WC_PK_TYPE_ED25519_{KEYGEN,SIGN,VERIFY}cases from#ifdef HAVE_CURVE25519inwh_Client_CryptoCbStd. - Adds a new refactor test that signs/verifies via wolfCrypt APIs using a server-only Ed25519 keyId (ensuring the cryptocb path is required).
- Introduces an
NOCURVE25519=1build knob for both test harness Makefiles and adds a CI job to exercise Ed25519-without-Curve25519.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/wh_client_cryptocb.c |
Moves the #endif /* HAVE_CURVE25519 */ to make Ed25519 dispatch cases compile independently of Curve25519. |
test-refactor/client-server/wh_test_crypto_ed25519.c |
Adds a regression test that forces Ed25519 sign/verify through the crypto callback using server-only keyIds. |
test/config/user_settings.h |
Allows disabling HAVE_CURVE25519 for tests via WOLFHSM_CFG_TEST_NO_CURVE25519. |
test/Makefile |
Adds NOCURVE25519=1 knob to define WOLFHSM_CFG_TEST_NO_CURVE25519. |
test-refactor/posix/Makefile |
Adds the same NOCURVE25519=1 knob for the refactor harness. |
.github/workflows/build-and-test-refactor.yml |
Adds a CI build/test row that runs with NOCURVE25519=1 to cover the fixed dispatch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #493
Scan targets checked: wolfhsm-crypto-bugs, wolfhsm-src
No new issues found in the changed files. ✅
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 8 total — 2 posted, 6 skipped
Posted findings
- [Medium] New regression test's dispatch path is ambient rather than pinned, and the only CI row that runs it is DMA=1 —
test-refactor/client-server/wh_test_crypto_ed25519.c:413-420 - [Low] WC_PK_TYPE_ED25519_KEYGEN dispatch remains uncovered by the new regression test —
test-refactor/client-server/wh_test_crypto_ed25519.c:403-411
Skipped findings
- [Medium] Legacy NOCURVE25519 knob has no CI row and no test that would catch the regression
- [Low] File-header test inventory not updated with the new test function
- [Low] New test duplicates ~60 lines of setup/teardown from _whTest_CryptoEd25519ServerKey
- [Low] New NOCURVE25519 knob in test/Makefile is never exercised by CI
- [Low] Regression test does not pin the cryptocb dispatch mode it is guarding
- [Info] File header comment enumerates test functions and is now stale
Review generated by Skoll via Claude/Codex
14edfbe to
70a9529
Compare
70a9529 to
c6d0360
Compare
Problem
In
wh_Client_CryptoCbStd, the#ifdef HAVE_ED25519block holding theWC_PK_TYPE_ED25519_KEYGEN,_SIGNand_VERIFYcases was nested inside the#ifdef HAVE_CURVE25519block. These are independent wolfSSL build options, sothe three dispatch cases compiled only when both were defined.
With Ed25519 enabled and Curve25519 disabled, every Ed25519 operation on a
wolfHSM devId fell through to
default: ret = CRYPTOCB_UNAVAILABLE;, whichwh_Client_CryptoCbdeliberately propagates, so wolfCrypt silently used itssoftware implementation. For a key whose private scalar lives only in the HSM
(
devCtxholds a keyId, no local material) that path cannot sign at all — HSMoffload broke with no diagnostic. The DMA sibling
wh_Client_CryptoCbDmaalready had the correct sibling layout, confirming the nesting was accidental.
Fix (
src/wh_client_cryptocb.c)Moved
#endif /* HAVE_CURVE25519 */up to just after theWC_PK_TYPE_CURVE25519case so the Ed25519 group is a sibling rather than a child:
One line moved, net zero. Every other Curve25519/Ed25519 guard pair in the repo
was audited and is already correct. Closes f-7149.
Test harness
The existing Ed25519 tests did not catch this: they call the
wh_Client_Ed25519*APIs directly, bypassing the crypto callback, and the inlinetest signs with a key that still holds local private material. The whole suite
passed with the dispatch cases compiled out.
_whTest_CryptoEd25519CryptoCbHsmKeysigns viawc_ed25519_sign_msg()on akey reduced to a bare keyId, so the callback must handle it; a software fallback
returns
BAD_FUNC_ARG. Pins the std path viawh_Client_SetDmaMode(ctx, 0)sothe guard it protects cannot be masked by ambient DMA mode.
NOCURVE25519=1build knob in both harness Makefiles, gatingHAVE_CURVE25519in the sharedtest/config/user_settings.h.build-and-test-refactor.ymlandbuild-and-test.yml.Verification
Negative control on unfixed source:
whTest_Crypto_Ed25519failsrc=-173(
BAD_FUNC_ARG) in bothNOCURVE25519configs and passes with the fix,isolating the defect to the guard. All builds clean under
-std=c90 -Werror -Wall -Wextra, ASan clean.NOCURVE25519=1 ASAN=1DMA=1 ASAN=1 NOCURVE25519=1DMA=1 ASAN=1ASAN=1DMA=1 ASAN=1, with and withoutNOCURVE25519=1Intentionally not in this PR, both tracked as follow-ups: mandatory-dispatch
coverage for
WC_PK_TYPE_ED25519_KEYGEN(needsWOLF_CRYPTO_CB_ONLY_ED25519,which would also force the inline test's local-material sign through the
callback), and cryptocb-API coverage of the
wh_Client_CryptoCbDmaEd25519 group.